Repository navigation
wallet: Keep Bails identifiers downstream - #80
Conversation
BenWestgate
added
cli
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ea2ed7093
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated release-gate review, posted at the maintainer request. Code ACK 9ea2ed7: the unavailable RIPEMD-160 path now reports an inconclusive standard-Bails check instead of asserting a mismatch; a matching Bails-alpha SHA-256 identifier still wins; and the no-record fingerprint display plus explicit operator confirmation are unchanged. The new frozen-vector regression passes, both current GitHub package matrices are green (12/12 jobs), and the isolated #42/#45/#7/#51/#57/#46 composition plus this commit passes 918 tests at 5,142/5,200 source review lines. No code blocker found. This is not an authorship ACK: the agent-authored commit needs responsible human review and rewrite/squash before merge, and the stack must be refreshed after #7/#57/#46 integration.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Exact-head release-gate recheck: ACK da14e0d. The prior P1 is resolved: the runtime now distinguishes an unavailable RIPEMD-160 check from a mismatch, still allows the Bails-alpha SHA-256 match to win, and both docs/security/model.md and invariant 4 describe that degraded outcome. git diff --check is clean; the focused identity/identifier tests pass in normal and python -O modes (9/9 each). No additional code blocker found. This remains stacked after #46 and needs the responsible human to review/rewrite or squash the agent-authored commits when the final #57/#46 integration is settled.
|
Agent release-gate re-review at exact head |
da14e0d to
0f5f4a8
Compare
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
|
AI-assisted release-gate recheck at exact head |
0f5f4a8 to
fb8d671
Compare
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Agent release-gate re-review: ACK fb8d671.
The refreshed runtime patch is unchanged from the reviewed Bails-availability fix, and the documentation conflict resolution preserves the current Core-native seed-source contract while adding the required “RIPEMD-160 unavailable = inconclusive, not mismatch” wording. The sole inline thread is resolved. Exact-head Python-package run 546 and Bitcoin Core wallet-fixture run 11 both succeeded; focused no-record/identity tests also passed normally and under python -O.
No remaining code-review blocker found. Integration/authorship policy remains: #80 follows #57 and its agent-authored commits should be human-reviewed/re-written or squashed before merge.
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: ACK 096b654. A missing RIPEMD-160 implementation is now represented as an unavailable standard-Bails check rather than a false mismatch, while the independent fingerprint confirmation and Bails-alpha SHA-256 check remain intact. The no-record fallback still requires explicit operator confirmation and does not treat the inconclusive identifier check as authentication. Exact-head Python-package and Bitcoin Core fixture CI are green and all review threads are resolved. No correctness or release blocker found.
096b654 to
177f176
Compare
When RIPEMD-160 is unavailable, a valid standard Bails identifier cannot be checked. Preserve the Bails-alpha SHA-256 result and distinguish that inconclusive state from a completed identifier mismatch, so the no-record restore prompt does not claim the cards are wrong. Keep the independent fingerprint and explicit operator-confirmation boundary unchanged. Refs #79
The no-record restore flow can no longer claim a standard Bails identifier mismatch when RIPEMD-160 is unavailable. Record that platform-dependent inconclusive outcome in both the security model and invariant so reviewers can distinguish it from a completed comparison. Refs #79
177f176 to
729cd24
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/crypto/ripemd160.py I'd prefer bails fall back to this version. It downloads the Bitcoin Source code anyhow so it should have it. And this seems like a Bails issue not a python-codex32 issue. There is no reason a python-codex32 user needs their seeds checked as if they were created with an old version of bails, so consider sending this patch downstream or something? coordinate with both projects to figure out how to handle a missing ripemd160. |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
cNACK, bails stuff should be in bails. Leave it out of python-codex32 since it doesn't create backups with this default identifier, we can at best show it for unshared secrets which DO have a default identifier. But remind it remains unconfirmed (that not matching the ID shown means the wrong seed was recovered) since users may set custom identifiers that override the default.
If their unshared identifier does match our fingerprint id default it's a 1/2^20 chance so a good assurance the correction is correct.
Remove Bails-specific seed-digest comparisons from the shared wallet adapter. A 20-bit fingerprint-derived identifier is a useful hint for an unshared default-identifier backup; a missing match is inconclusive for custom identifiers or shares. Preserve the typed fingerprint gate. Refs #79
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BenWestgate
changed the title
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: ACK 8fa27ef. Removed legacy Bails identifier checks and their tests from this library. The remaining 20-bit default-identifier match is explicitly inconclusive; typed fingerprint verification is unchanged. Reviewed the five-file diff; no blocker found. Bails #351 tracks legacy handling.
|
I don't see the downstream patch/issue, can you link to it? |
|
AI-assisted follow-up: downstream Bails identifier handling is tracked in Bails #351. That issue remains open; its RIPEMD-160-unavailable case must remain inconclusive, not a mismatch. No downstream patch has been merged. |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Apply the review suggestions in #80. State the fresh-creation recording requirement directly and remove redundant Bails and executable-review qualifiers from the security model. Runtime behavior is unchanged.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted code/security review: ACK df3559e. Five-file PR diff reviewed; the legacy Bails checks are removed, and a 20-bit identifier match remains only an inconclusive hint. The fingerprint gate is unchanged. Both exact-head CI workflows pass; the separate 11-file frozen-range security scan found no reportable issue. All review threads are resolved.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Why
Bails' legacy seed-digest identifiers belong in Bails integration. A no-record restore in python-codex32 cannot infer that custom or shared identifiers indicate bad cards.
Change
Compare only the 20-bit fingerprint-derived default used by unshared codex32 secrets. A match is a useful hint, not wallet authentication; a nonmatch is inconclusive. Remove RIPEMD-160/SHA-256 legacy checks and update the security model and tests. The typed fingerprint gate and explicit no-record choice are unchanged.
Validation
290 Bitcoin Core and CLI tests passed on Python 3.14 with an isolated local editable installation. Relevant focused tests also passed under
python -O;git diff --checkis clean. Exact-head GitHub CI must complete before merge.Review
#81 remains stacked on this branch and requires revalidation against its new base. Commit authorship and final human review remain governed by CONTRIBUTING.md.
Refs #79, #81.